-
Notifications
You must be signed in to change notification settings - Fork 74
[flags] Generic resolve method #2923
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: feature/feature-flagging
Are you sure you want to change the base?
[flags] Generic resolve method #2923
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feature/feature-flagging #2923 +/- ##
============================================================
+ Coverage 70.76% 70.85% +0.08%
============================================================
Files 835 840 +5
Lines 30388 30554 +166
Branches 5132 5163 +31
============================================================
+ Hits 21504 21647 +143
- Misses 7431 7451 +20
- Partials 1453 1456 +3
🚀 New features to boost your workflow:
|
2b5bcf6
to
211e111
Compare
77e34af
to
287fa8a
Compare
# Conflicts: # features/dd-sdk-android-flags/src/main/kotlin/com/datadog/android/flags/featureflags/FlagsClient.kt # features/dd-sdk-android-flags/src/main/kotlin/com/datadog/android/flags/featureflags/internal/DatadogFlagsClient.kt # features/dd-sdk-android-flags/src/test/kotlin/com/datadog/android/flags/featureflags/internal/DatadogFlagsClientTest.kt
be2f0e8
to
626593b
Compare
is Int -> "Int" | ||
is Double -> "Double" | ||
is JSONObject -> "JSONObject" | ||
else -> defaultValue!!::class.simpleName ?: "Unknown" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Theoretically someone could pass null for T. In the current codepaths this would possibly be blocked at isTypeCompatible
, but I think it's probably better to avoid the !!. We could do something like:
defaultValue?.let { it.class.simpleName ?: "Unknown" } ?: "Unknown"
or even directly return "Unknown" depending how important it is to know the exact class name.
fun resolveDoubleValue(String, Double): Double | ||
fun resolveIntValue(String, Int): Int | ||
fun resolveStructureValue(String, org.json.JSONObject): org.json.JSONObject | ||
fun <T> resolve(String, T): com.datadog.android.flags.model.ResolutionDetails<T> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specification says that even for concrete methods above we need to return ResolutionDetails
https://openfeature.dev/specification/sections/providers, but we are returning just the objects. Is it fine to not use this structure there?
* @param defaultValue The value to return if the flag cannot be retrieved or parsed. | ||
* @return [ResolutionDetails] containing the value, variant, reason, error info, and metadata. | ||
*/ | ||
fun <T> resolve(flagKey: String, defaultValue: T): ResolutionDetails<T> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we support nullable types (values) here? If it is not the case, type should be T : Any
* @param flagKey The name of the flag to query. Cannot be null. | ||
* @param defaultValue The value to return if the flag cannot be retrieved. | ||
* @return The string value of the flag, or the default value if unavailable. | ||
* @param defaultValue The value to return if the flag cannot be found resolved for any reason. Cannot be null. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* @param defaultValue The value to return if the flag cannot be found resolved for any reason. Cannot be null. | |
* @param defaultValue The value to return if the flag cannot be found or resolved for any reason. Cannot be null. |
private fun <T> resolveValue(flagKey: String, defaultValue: T): T = | ||
resolveTracked(resolveInternal(flagKey, defaultValue)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is confusing: here we have 3 resolveXXX
methods, and the fact that there is resolveInternal
with a result passed to resolveTracked
doesn't add clarity. Should resolveInternal
be renamed, maybe?
* @param defaultValue The value to return if the flag cannot be retrieved or parsed. | ||
* @return [ResolutionDetails] with either the parsed value and metadata, or an error. | ||
*/ | ||
override fun <T> resolve(flagKey: String, defaultValue: T): ResolutionDetails<T> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor: it is better to move this up, so that we have interface methods grouped first and only after them we have private
methods.
* @param extraLogging The JSONObject containing additional flag metadata. | ||
* @return An immutable map of metadata, or null if the JSONObject is empty. | ||
*/ | ||
private fun extractMetadata(extraLogging: JSONObject): Map<String, Any>? { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need to explicitly make return type nullable? Cannot we return empty collection if JSONObject
is empty?
) { | ||
// Catch all conversion exceptions (including JSONException) and return null | ||
// The caller is responsible for logging errors as appropriate | ||
null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe it is worth to log it with MAINTAINER
target?
fun <T> isTypeCompatible(variationType: String, defaultValue: T): Boolean = when (defaultValue) { | ||
is Boolean -> variationType == VariationType.BOOLEAN.value | ||
is String -> variationType == VariationType.STRING.value | ||
is Int -> variationType == VariationType.INTEGER.value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we also check for VariationType.NUMBER
here as below?
@Suppress("UNCHECKED_CAST") | ||
private fun <T> getConverterForType(defaultValue: T): (String) -> T? = when (defaultValue) { | ||
is Boolean -> { s: String -> s.lowercase(Locale.US).toBooleanStrictOrNull() as? T } | ||
is String -> { s: String -> s as? T } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this one can be just { s: String -> s }
: input and output types are the same, to cast is not needed
DOUBLE("double"), | ||
JSON("json") | ||
NUMBER("number"), | ||
FLOAT("float"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we still have the usage of double
term in API, should we keep it here? Or is it the change per spec?
* @property flagMetadata Optional map of arbitrary metadata associated with the flag (string keys, primitive values). | ||
*/ | ||
data class ResolutionDetails<T>( | ||
val value: T, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can value be null? If not, it is better to do data class ResolutionDetails<T: Any>
if (flagsConfiguration.rumIntegrationEnabled && resolution.flag.doLog) { | ||
logEvaluation( | ||
key = resolution.flagKey, | ||
value = resolution.value as Any |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If value is never null, you should make T: Any
everywhere like I described here https://github.com/DataDog/dd-sdk-android/pull/2923/files#r2444011674
/** | ||
* Error codes for flag resolution failures, aligned with the OpenFeature specification. | ||
*/ | ||
enum class ErrorCode { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only see that FLAG_NOT_FOUND
, PARSE_ERROR
and TYPE_MISMATCH
are used in this PR. Will others be used in other PRs or are they not needed?
* @param T The type of the resolved flag value (Boolean, String, Int, Double, JSONObject). | ||
* @property value The resolved flag value. This is always present, either from flag evaluation or the default value. | ||
* @property variant Optional string identifier for the resolved variant (e.g., "control", "treatment"). | ||
* @property reason Optional string explaining why this value was resolved (e.g., "TARGETING_MATCH", "DEFAULT", "ERROR"). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e.g., "TARGETING_MATCH", "DEFAULT", "ERROR"
Is it possible to make this a enum instead of a String?
* @param defaultValue The default value (used to infer the target type) | ||
* @return The converted value, or null if types are incompatible or conversion fails | ||
*/ | ||
@Suppress("UNCHECKED_CAST") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Suppress("UNCHECKED_CAST")
this is not needed here
...-flags/src/main/kotlin/com/datadog/android/flags/featureflags/internal/FlagValueConverter.kt
Outdated
Show resolved
Hide resolved
* @return The converted value, or null if types are incompatible or conversion fails | ||
*/ | ||
@Suppress("UNCHECKED_CAST") | ||
fun <T> convert(variationValue: String, variationType: String, defaultValue: T): T? { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit
You don't actually need a defaultValue here. You can pass the class itself using KClass
.
internal object FlagValueConverter {
fun <T : Any> convert(variationValue: String, variationType: String, resultType: KClass<T>): T? {
if (!isTypeCompatible(variationType, resultType)) {
return null
}
return try {
doConvert(variationValue, resultType)
} catch (
@Suppress("TooGenericExceptionCaught", "SwallowedException")
e: Exception
) {
null
}
}
fun isTypeCompatible(variationType: String, resultType: KClass<*>): Boolean = when (resultType) {
Boolean::class -> variationType == VariationType.BOOLEAN.value
String::class -> variationType == VariationType.STRING.value
Int::class -> variationType == VariationType.INTEGER.value
Double::class -> variationType == VariationType.NUMBER.value || variationType == VariationType.FLOAT.value
JSONObject::class -> variationType == VariationType.OBJECT.value
else -> false
}
@Suppress("UNCHECKED_CAST")
private fun <T : Any> doConvert(s: String, resultType: KClass<T>): T? = when (resultType) {
Boolean::class -> s.lowercase(Locale.US).toBooleanStrictOrNull() as? T
String::class -> s as? T
Int::class -> s.toIntOrNull() as? T
Double::class -> s.toDoubleOrNull() as? T
JSONObject::class -> JSONObject(s) as? T
else -> null
}
fun getTypeName(type: KClass<*>): String = when (type) {
Boolean::class -> "Boolean"
String::class -> "String"
Int::class -> "Int"
Double::class -> "Double"
JSONObject::class -> "JSONObject"
else -> type.simpleName ?: "Unknown"
}
}
PR #2923: [flags] Generic resolve method
What does this PR do?
This PR implements the generic
resolve<T>()
method providing OpenFeature-compatible flag resolution with detailed error information, metadata, and variant tracking.Key additions:
public API
resolve<T>(flagKey, defaultValue)
method that returnsResolutionDetails<T>
ResolutionDetails<T>
data class containing value, variant, reason, error code, error message, and flag metadataErrorCode
enum with OpenFeature-spec error codes (FLAG_NOT_FOUND, TYPE_MISMATCH, PARSE_ERROR, etc.)internal
FlagValueConverter
object for centralized, side-effect-free type conversionInternalResolution
sealed classMotivation
OpenFeature Specification Alignment
The OpenFeature specification defines a
resolve()
method as the core evaluation API, providing rich resolution details beyond just the flag value. This enables:Additional Notes
Anything else we should know when reviewing?
Review checklist (to be filled by reviewers)